Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add gd go to definition motion #258

Merged

Conversation

alisonatwork
Copy link
Collaborator

@alisonatwork alisonatwork commented Jan 28, 2020

This is a new attempt to add the VS Code "go to definition" command as the gd amVim motion.

In the previous approach (#250) i implemented the command as a top-level amVim action, but this caused problems because it hijacked the matching of motions that already existed, such as gg. I looked at several ways to solve this, but i think the most vim-like was to respect the concept of gd as a motion, so that's what i have attempted here.

It required three steps:

  1. Change the Motion.apply function to be async (i.e. return a promise).
    This was required because in order to make motions able to execute VS Code commands, they need to be able to wait for the Thenable<boolean> to resolve. This problem was also encountered by @Ddedalus in Draft method motions #230, so adding this functionality should also help enable the [ and ] commands. This change does make the code on the top level a bit less pretty, however. (See ActionDelete in particular.)
  2. Add the new gd (and gD) commands.
    These commands execute the action inside the application of the motion, then return the resulting cursor position. This works great if you are inside the same file - i.e. dgd and ygd do exactly what you would expect. Inside a different file, they can cause strange and unexpected blocks to be deleted or yanked, but that is consistent with the behavior of actual vim, so i think it's okay.
  3. Allow language to be set on test cases.
    In order for the language server features to work (go to definition etc) any new text document must have a language id set to something other than plaintext (the default). The problem i found is that loading the language server has some lag, and the integration tests were not reliably passing. So now i sleep for a while after opening the first document marked as a particular language.

Note: this supersedes #257

@alisonatwork
Copy link
Collaborator Author

@karlhorky I would love to get you to weigh in on this PR. In particular, I am interested in your thoughts about the version in current head (fad820d) and in head~1 (1f6152a). The main difference between the two is I have refactored some of the code to be more procedural using await/await/await instead of trying to push everything into functional style reduce/then/then/then.

On one hand, this procedural approach goes against a lot of the existing coding style which is more functional. On the other hand, when i made the motion async, it made applying the motion to a list of selections kinda messy.

This is a much-desired feature from me, but i've been through at least one iteration of this that we had to revert due to the original solution hijacking a key used in a different mode, so i'd like to be sure that when we merge it this time it really is good.

@karlhorky
Copy link
Contributor

Sure, I'll plan to dig in to this tomorrow.

@karlhorky
Copy link
Contributor

karlhorky commented Jun 9, 2020

Ok, so I reviewed your two commits, and here's some feedback:

  1. The change from .then-chaining to async/await looks great! Lots of nesting and callback hell reduced.
  2. My feelings are mixed on the change from the functional style to imperative / procedural. On the first hand, a functional style can make things easier to reason about and avoid having too much state to keep in your head. But, on the other hand, as you have probably found, array methods such as map and reduce don't play very nicely with async/await and promises (requiring extra manual unwrapping), and in this case I think the imperative version is probably less clumsy and worth the downsides.

So, I think this looks good :)

@alisonatwork
Copy link
Collaborator Author

Thanks for your review @karlhorky - I also am in two minds about it, because i like the purity of functional style, but given there are more and more places where we do need to await things, i think it's nicer to be consistently procedural, even if it looks a bit odd to await/await/await on functions that in reality are synchronous.

Did you also give the feature a manual run-through? The tests are all passing on my machine, but the Github actions intermittently fail, i think because of slow loading of the language server. I am thinking about bumping the Mocha retry to 3 before merging this just to see if that will make the tests more robust.

If you're happy, i think we can merge this and ask for another release to the marketplace. Then we can tackle the last outstanding PR, which might need a few more updates to get into shape, plus whatever new features seem cool.

@karlhorky
Copy link
Contributor

karlhorky commented Jun 11, 2020

Yeah I think that your changes look good, so I'm good with you merging whenever you like.

Didn't get a chance to test this yet, maybe on the weekend... Is there a recommended way that you recommend for installing / testing the development version of the extension? With the other feature that I did, I just pasted the code into the live extension 😅 but I guess there are probably better ways.

@alisonatwork
Copy link
Collaborator Author

Let's merge on the weekend after you've given it a run through, i'd like to be sure it works on someone else's machine too 😄

There is an easier way to test than copying the files across - if you open the project inside VS Code, in the run configurations there are two options: "Extension" and "Extension Tests". If you run the former, it will pop open a new, sandboxed VS Code which is running the extension. The latter zooms through all the tests, which now with Webpack and the newer test framework we merged a couple weeks ago should only take about a minute to do.

In the mean time i will play around with Mocha retry and some other settings to see if i can get the CI consistently passing.

Thanks for the help!

@karlhorky
Copy link
Contributor

Ah ok, you just would like me to run the automated tests, I understand. Yeah I can give that a go.

@alisonatwork
Copy link
Collaborator Author

alisonatwork commented Jun 11, 2020

Running the automated tests would be cool too, although hopefully the CI can cover that for us now. I'm more interested in if you can give the PR a test run with real code, try navigate around with gd/gD, and also give it a go as a motion. I'm not on my computer at the mo to confirm but stuff like dgd and cgd should also work with this implementation. I run Windows 10 at home and my "vim fu" is limited to certain key presses (for example I never use f) so it'd be good to get some input from someone who may have a different environment or workflow.

@karlhorky
Copy link
Contributor

Sorry, didn't get around to this on the weekend... still on my list though, and will try for sometime in the next 3-5 days.

@alisonatwork
Copy link
Collaborator Author

alisonatwork commented Jun 20, 2020

No probs. I am on vacation next week, so i plan to get this merged soon and see if there are any other low hanging fruits we could do, then hopefully get a new release out by Dragon Boat Festival 🚀

@karlhorky
Copy link
Contributor

karlhorky commented Jun 21, 2020

if you open the project inside VS Code, in the run configurations there are two options: "Extension" and "Extension Tests"

Ok, I did this:

  1. For Extension, I tried gd and gD with the function calls in two files (see the following code), and they seemed to work :) Admittedly, I don't use these chords in Vim, so I'm not sure what it's supposed to do, but both gd and gD took me to the definition in the respective file.
  2. For Extension Tests, I got 492 passing (57s), which I assume means that none failed.
// 1.js
import defaultt, { eeee } from './2';
function asdf() {}
asdf();
defaultt();
eeee();
// 2.js
export function eeee() {}
export default function defaultt() {}

And cool about the Dragon Boat Festival - do you paddle? I used to do that semi-competitively on the west coast in Canada.

@alisonatwork
Copy link
Collaborator Author

Thanks for the review! I'll merge this and see if we can get a release.

On Dragon Boat Festival: i don't paddle, i just live in China 😁 I'm thinking of moving back to Canada soon so i'll have to update my holiday references!

@karlhorky
Copy link
Contributor

No worries, sounds good! Thanks :)

@alisonatwork alisonatwork deleted the add-go-to-definition-motion branch March 21, 2021 23:02
@alisonatwork alisonatwork restored the add-go-to-definition-motion branch March 21, 2021 23:02
@alisonatwork alisonatwork deleted the add-go-to-definition-motion branch March 21, 2021 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants